-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tools] introduce circle plus generator #12977
Conversation
483bf38
to
9237f7d
Compare
|
||
## Inject training parameters using json file | ||
|
||
<!--to be updated --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this feature in next PR
47ea7c2
to
1926ede
Compare
This introduces a circle plus generator, which helps handle a circle file with training hyperparameters. As a first feature, it checks whether a circle file contains training parameters or not. ONE-DCO-1.0-Signed-off-by: seunghui youn <sseung.youn@samsung.com>
1926ede
to
566d5e2
Compare
@zetwhite Could you reduce example model file size? (under 5KB if possible) BTW, I failed to open example model on
|
Good~ This PR looks good to me :) The attached (added) And the details
correct file
|
cfa2edd
to
7ffa2ac
Compare
I'm still updating this PR. I'll re-request the review explicitly if it is ready. |
b99872e
to
b252485
Compare
@jyoungyun @ragmani I updated PR based on code reviews. Could you take another look? @mhs4670go Thank you a lot for the detailed reviews! Thanks to your review(especially about SRP rules), I could get a much better code. Pleas take another look. |
b252485
to
a4246b8
Compare
tools/circle_plus_gen/README.md
Outdated
|
||
2. Install required pakcages. | ||
|
||
Currently, only `flatbuffers==24.3.25` is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, only `flatbuffers==24.3.25` is needed. | |
Currently, only `flatbuffers` is needed. |
I think this tool does not require a specific flatbuffers version, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this tool does not require a specific flatbuffers version, doesn't it?
Maybe.
But I couldn't guarantee that it works well with other versions.
So, How about
- removing the version in README.md (because we don't have to emphasize the version in readme)
- keeping the version in requirements.txt (because specifying the working well version is necessary)
The attached
|
''' | ||
print(f"check hyperparameters in {in_circle_file}") | ||
|
||
circle_model: CirclePlus = CirclePlus.from_file(in_circle_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to write the type explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No special reason.
I thought that.. writing type here helps the reader not to read the inside of CirclePlus.from_file
.
044d78a
to
611daaa
Compare
611daaa
to
58364bf
Compare
@jyoungyun sorry for your double-checks. There might be no problem this time. I uploaded it again and thank you for your thoughtful review. 😄
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jyoungyun sorry for your double-checks. There might be no problem this time. I uploaded it again and thank you for your thoughtful review. 😄
This file works well. This code looks good to me. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@hseok-oh could you merge this one? |
# | ||
# * flatc version 23.5.26 is used | ||
# ./flatc --python --gen-onefile --gen-object-api ../../../nnpackage/schema/circle_schema.fbs | ||
# ./flatc --python --gen-onefile --one-object-api ../../../runtime/libs/circle-schema/include/circle_traininfo.fbs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# ./flatc --python --gen-onefile --one-object-api ../../../runtime/libs/circle-schema/include/circle_traininfo.fbs | |
# ./flatc --python --gen-onefile --gen-object-api ../../../runtime/libs/circle-schema/include/circle_traininfo.fbs |
typo again.. I'll fix it in next PR.
This introduces a circle plus generator, which helps handle a circle file with training hyperparameters.
As a first feature, it checks whether a circle file contains training parameters or not.
ONE-DCO-1.0-Signed-off-by: seunghui youn sseung.youn@samsung.com